Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BlockHound integration #6692

Closed
wants to merge 1 commit into from

Conversation

bsideup
Copy link

@bsideup bsideup commented Oct 29, 2019

BlockHound is a tool for JVM to detect blocking calls in non-blocking threads.

It provides an SPI to integrate with, although it also comes with built-in RxJava2 integration. Since the introduction of SPI, we encourage JVM frameworks to provide an implementation of the SPI.
Netty project just added such integration, and even Project Reactor itself uses the SPI to integrate with BlockHound.
The main motivation is to be able to adjust the integration if the internals are changed, e.g. if RxJava decides to use some other marker interface for its threads.

BlockHound is Java 8+, but the SPI class is only loaded when BlockHound installs the integrations (using Java's ServiceLoader), hence there should be no problem with RxJava3's baseline (which is Java 6 at the moment). There are no Java 8 bytecodes in that class too, only JDK 8 class references. Netty is Java 6 too and it was fine for them.

A separate test source is used because BlockHound is global (since it is implemented as a Java agent) and cannot be disabled after the installation.

@akarnokd
Copy link
Member

No. I don't want to add Blockhound integration.

@akarnokd akarnokd closed this Oct 29, 2019
@lanwen
Copy link

lanwen commented Oct 29, 2019

@akarnokd just tested this helper with micronaut and found it quite useful, can I ask the reason of rejection?

@bsideup
Copy link
Author

bsideup commented Oct 29, 2019

@akarnokd I understand that you're busy, but will appreciate a bit longer answer, since I believe that the community will benefit from RxJava3 getting a support for BlockHound, which is (despite the organization) is neutral, does not require you to have a dependency on it and does not add any performance overhead to the framework (unless the agent is explicitly enabled by the user, and it is a very low one, there are some public benchmarks too)

@akarnokd
Copy link
Member

The PR code demonstrates there is no need to add BlockHound to the library directly; any external companion library can setup that Thread identification. Also the PR doesn't work because the BlockHound API requires Java 8 which we don't support and may not support for months, but as an external support, users can rely on early-access desugaring on Android to workaround.

@bsideup
Copy link
Author

bsideup commented Oct 29, 2019

@akarnokd ok, thank you for clarifying the rejection, I respect your decision as a maintainer.

bsideup added a commit to reactor/BlockHound that referenced this pull request Nov 6, 2019
Since [RxJava have decided to not accept the BlockHound integration](ReactiveX/RxJava#6692),
we should clarify what users of the future versions of RxJava should do
to enable BlockHound with RxJava.

If RxJava community decide to add an integration with BlockHound,
we will update it accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants